Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Workaround for issue #3642. #3670

Merged
merged 3 commits into from
Jul 15, 2019
Merged

Workaround for issue #3642. #3670

merged 3 commits into from
Jul 15, 2019

Conversation

martinmr
Copy link
Contributor

@martinmr martinmr commented Jul 15, 2019

For some reason, there are nil values stored in the uidMatrix, which
results in a segmentation fault when trying to access the Uids field.
The root cause of this issue is still unclear but this change adds an
additional check to prevent the segmentation fault.


This change is Reviewable

For some reason, there are nil values stored in the uidMatrix, which
results in a segmentation fault when trying to access the Uids field.
The root cause of this issue is still unclear but this change adds an
additional check to prevent the segmentation fault.
query/groupby.go Outdated Show resolved Hide resolved
Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: Got a comment.

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @martinmr)


query/groupby.go, line 230 at r2 (raw file):

					continue
				}
				for _, uid := range ul.Uids {

Fix is good. But, I wonder: Can you change this to ul.GetUids() instead? Would the effect be the same?

Copy link
Contributor Author

@martinmr martinmr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @manishrjain and @martinmr)


query/groupby.go, line 230 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Fix is good. But, I wonder: Can you change this to ul.GetUids() instead? Would the effect be the same?

I think so. This is the definition of GetUids:

func (m *List) GetUids() []uint64 {
	if m != nil {
		return m.Uids
	}
	return nil
}

I'll make the change.

@martinmr martinmr merged commit e1efaa3 into master Jul 15, 2019
@martinmr martinmr deleted the martinmr/fix-3642 branch July 15, 2019 21:52
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 19, 2019
For some reason, there are nil values stored in the uidMatrix, which
results in a segmentation fault when trying to access the Uids field.
The root cause of this issue is still unclear but this change adds an
additional check to prevent the segmentation fault.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants